Stabilize path for libusb devices: #406 from signal11/hidapi#117
Stabilize path for libusb devices: #406 from signal11/hidapi#117gavincangan wants to merge 4 commits into
Conversation
Youw
left a comment
There was a problem hiding this comment.
please follow this document code style:
replace all spaces with tabs, and, I think, we good to go
Co-Authored-By: Ihor Dutchak <ihor.youw@gmail.com>
|
HI. What is the status of this PR? |
|
soon to be merged |
|
is there a good reason to pack two levels into a 16bit hex number instead of separating each level with a colon? |
|
@bearsh IMHO, to make it look more like an IPv6 address? People are already used to seeing quad-hex strings, so it makes sense to keep it that way. It also shortens the string a little. I suppose you COULD have a byte-order problem, but these strings are not portable across machines so it shouldn't matter. I find it very improbable that you have one machine that changes byte-order. |
|
@derekatkins valid reason... and in the end I don't really care as long as all levels are in the string just some thoughts:
|
This sounds like execent reasoning |
|
@Youw what's your plan on this? do you expect a change in the PR? |
|
The more I think about this, the more I like the idea to have device path look as much similar to what linux kernel have as possible. Even though current PR gives an improvement, I wouldn't want this to be changed more than once. Does anyone can reference a linux kernel implementation for this part? |
I've found following sites 'talking' about the usb sysfs interface
the most authoritative entry may be the following line from the ABI doc (direct link):
|
|
Very good. I'm pretty sure that |
|
if it should be like |
|
Right. I answered this (my) morning in a hurry. Didn't pay attention to all the details. I say we try to do exactly same way as Linux kernel does: So we need to include configuration number/index too. A also believe, that we shouldn't include all 7 port numbers (as max supported by USB3.0), but only as much as reported by |
I agree with this statement. It should only display the number of ports in use. |
|
something like this? diff --git a/hidapi/libusb/hid.c b/hidapi/libusb/hid.c
index 0b8aa1e..3d71767 100644
--- a/hidapi/libusb/hid.c
+++ b/hidapi/libusb/hid.c
@@ -484,15 +484,29 @@ err:
return str;
}
-static char *make_path(libusb_device *dev, int interface_number)
+static char *make_path(libusb_device *dev, int interface_number, int config_number)
{
- char str[64];
- snprintf(str, sizeof(str), "%04x:%04x:%02x",
- libusb_get_bus_number(dev),
- libusb_get_device_address(dev),
- interface_number);
- str[sizeof(str)-1] = '\0';
-
+ char str[64]; // max length "000-000.000.000.000.000.000.000:000.000"
+ // Note that USB3 port count limit is 7; use 8 here for alignment
+ uint8_t port_numbers[8] = {0, 0, 0, 0, 0, 0, 0, 0};
+ int num_ports = libusb_get_port_numbers(dev, port_numbers, 8);
+
+ if (num_ports > 0) {
+ int n = snprintf(str, sizeof("000-000"), "%u-%u", libusb_get_bus_number(dev), port_numbers[0]);
+ for (uint8_t i = 1; i < num_ports; i++) {
+ n += snprintf(&str[n], sizeof(".000"), ".%u", port_numbers[i]);
+ }
+ n += snprintf(&str[n], sizeof(":000.000"), ":%u.%u", (uint8_t)config_number, (uint8_t)interface_number);
+ str[n] = '\0';
+ } else {
+ // USB3.0 specs limit number of ports to 7 and buffer size here is 8
+ if (num_ports == LIBUSB_ERROR_OVERFLOW) {
+ LOG("make_path() failed. buffer overflow error\n");
+ } else {
+ LOG("make_path() failed. unknown error\n");
+ }
+ str[0] = '\0';
+ }
return strdup(str);
}
@@ -590,7 +604,7 @@ struct hid_device_info HID_API_EXPORT *hid_enumerate(unsigned short vendor_id,
/* Fill out the record */
cur_dev->next = NULL;
- cur_dev->path = make_path(dev, interface_num);
+ cur_dev->path = make_path(dev, interface_num, conf_desc->bConfigurationValue);
res = libusb_open(dev, &handle);
@@ -915,7 +929,7 @@ hid_device * HID_API_EXPORT hid_open_path(const char *path)
const struct libusb_interface_descriptor *intf_desc;
intf_desc = &intf->altsetting[k];
if (intf_desc->bInterfaceClass == LIBUSB_CLASS_HID) {
- char *dev_path = make_path(usb_dev, intf_desc->bInterfaceNumber);
+ char *dev_path = make_path(usb_dev, intf_desc->bInterfaceNumber, conf_desc->bConfigurationValue);
if (!strcmp(dev_path, path)) {
/* Matched Paths. Open this device */
|
|
by the way, according to my running system it's not |
format the path like the linux kernel does: <busnum>-<port[.port]>…:<config num>.<interface num> See also libusb#117 Closes: libusb#117
|
I've created a new pull request for this: #291 |
|
Locking this thread if favour of #291 |
format the path like the linux kernel does: <busnum>-<port[.port]>…:<config num>.<interface num> See also libusb#117 Closes: libusb#117
Format the path like the linux kernel does: <busnum>-<port[.port]>…:<config num>.<interface num> Closes: #117
MIgrating @derekatkins 's changes from this PR: signal11/hidapi#406
Referenced by this issue: signal11/hidapi#405 in signal11/hidapi